Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

xcm-builder: describe local location #2715

Closed
wants to merge 1 commit into from
Closed

xcm-builder: describe local location #2715

wants to merge 1 commit into from

Conversation

muharem
Copy link
Contributor

@muharem muharem commented Dec 14, 2023 •
edited
Loading

DescribeLocation implementation for local xcm locations.

Currently, our runtimes lack a conversion mechanism for translating local root/pallet/plurality xcm locations into account IDs. However, there are instances where this functionality is needed - polkadot-fellows/runtimes#125.

In this PR, I propose three options to address this issue:

  1. Include (0, Here) description into the DescribeFamily type for the root location. Utilize terminals like DescribePalletTerminal and DescribeBodyTerminal independently for the remaining locations without any additional prefixes.
  2. Introduce a type similar to DescribeFamily called DescribeLocal.
  3. Introduce a more generic type called DescribeWithPrefix.

@muharem muharem requested a review from a team as a code owner December 14, 2023 17:02
@muharem muharem added the T6-XCM This PR/Issue is related to XCM. label Dec 14, 2023
@gavofyork
Copy link
Member

gavofyork commented Dec 15, 2023 •
edited
Loading

The Plurality and pallet instance stuff is probably fine (though plurality might better be described with readable language rather than just encoding some value (which isn't necessarily expected to stay the same between XCM versions)).

The rest totally misses the point. What has been added is merely a confusing duplicate of DescribeTerminus.

@muharem
Copy link
Contributor Author

muharem commented Dec 15, 2023

The Plurality and pallet instance stuff is probably fine (though plurality might better be described with readable language rather than just encoding some value (which isn't necessarily expected to stay the same between XCM versions)).

The rest totally misses the point. What has been added is merely a confusing duplicate of DescribeTerminus.

I see, I wasn't certain whether to utilize DescribeTerminus and other Terminals for local locations without a prefix. Specifically, DescribeTerminus results an empty byte slice. And DescribeLocal was intended to be used in conjunction with existing terminals, only to have a prefix.

However, if I can simply use DescribeTerminus to describe (0, Here), then we do not need any of this.

@muharem muharem closed this Dec 15, 2023
@gavofyork
Copy link
Member

(And actually, we already have DescribePalletTerminal for pallet instances.)

@muharem
Copy link
Contributor Author

muharem commented Dec 15, 2023

(And actually, we already have DescribePalletTerminal for pallet instances.)

yes, and for pluralities - DescribeBodyTerminal. I meant only to add LocalChain prefix to them, something like,

  • DescribeLocal<(DescribeTerminus, DescribePalletTerminal, DescribeBodyTerminal)>
  • or DescribeWithPrefix<.., b"LocalChain", (DescribeTerminus, DescribePalletTerminal, DescribeBodyTerminal)>

but never mind, without prefix also looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
T6-XCM This PR/Issue is related to XCM.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@muharem @gavofyork